feat(datastructures, binary-tree): connect all siblings #150
Conversation
BrianLusina
added
enhancement
Algorithm
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughAdds sibling-connection support to binary trees: a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In @datastructures/trees/binary/node.py:
- Around line 41-45: Fix the typo in the doc comment for the BinaryTreeNode.next
attribute: replace "If this node is the last node in the tree on the last node"
with "If this node is the last node in the tree on the last level". Edit the
comment block above self.next (referencing BinaryTreeNode and the next
attribute) so the wording is corrected while keeping the surrounding explanatory
sentences intact.
- Around line 193-194: The current BinaryTreeNode.__repr__ includes full object
references for parent and next which can cause infinite recursion; change
__repr__ to avoid recursing into those nodes by using only safe identifiers
(e.g., parent.key or parent.data or parent is None, and similarly
next.key/next.data) or omit parent/next entirely; update the __repr__
implementation in the BinaryTreeNode class to format parent and next as simple
scalars (like their key or None) instead of embedding self.parent/self.next
objects.
In @datastructures/trees/binary/README.md:
- Line 84: Fix the typo in the constraint line that currently reads "1000 <=
`node.data` <= 1000" by adding the missing negative sign so it correctly states
"-1000 <= `node.data` <= 1000"; update the constraint text in the README entry
referencing node.data to include the leading minus on the first 1000 to reflect
the intended range.
In @datastructures/trees/binary/test_utils.py:
- Around line 188-226: The expected tree in CONNECT_ALL_SIBLINGS_TEST_CASES has
incorrect left/right children (nodes like data=50 wrongly show
left=300/right=10), so update the test to either (a) correct the expected
BinaryTreeNode instances so their left/right subtrees exactly match the input
tree and only the next pointers are set, or (b) replace the structural equality
check with a traversal-based assertion: call connect_all_siblings(root) and walk
the resulting .next chain to assert the visitation order [100, 50, 200, 25, 75,
300, 10] and that the final .next is None; reference BinaryTreeNode and
connect_all_siblings when making the change.
- Around line 229-240: The tests in ConnectAllSiblingsTestCase mutate the input
trees (next pointers), causing cross-test pollution when parameterized.expand
reuses the same root objects; fix by cloning or reconstructing a fresh tree for
each test invocation (e.g., deep copy the provided root or rebuild from a
serialized representation) inside both test_connect_all_siblings and
test_connect_all_siblings_ptr before calling connect_all_siblings or
connect_all_siblings_ptr so each test operates on an unmodified tree.
In @datastructures/trees/binary/utils.py:
- Line 74: The parameter type for connect_all_siblings should be
Optional[BinaryTreeNode] because the function checks for None at the start;
update the function signature of connect_all_siblings to accept
Optional[BinaryTreeNode] (matching connect_all_siblings_ptr) and ensure Optional
is imported/available in the module so the type hint is consistent with the
runtime None handling.
🧹 Nitpick comments (2)
datastructures/trees/binary/tree/binary_tree.py (1)
89-92: Consider preserving type parameterTinstead ofAny.Changing from
List[List[T]]toList[List[Any]]reduces type safety. If the intent is to accommodate mixed types, consider usingobjector a union type instead, or document whyAnyis needed here.datastructures/trees/binary/node.py (1)
20-20: Note:nextshadows Python built-in.Using
nextas a parameter name shadows the built-innext()function. This is a common trade-off when the domain name is descriptive (like linked listnextpointers). Just be aware this prevents using the built-in within this scope.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (19)
datastructures/trees/binary/images/examples/connect_all_siblings_of_binary_tree_example_1.pngis excluded by!**/*.pngdatastructures/trees/binary/images/examples/connect_all_siblings_of_binary_tree_example_2.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/connect_all_siblings_of_binary_tree_solution_1.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/connect_all_siblings_of_binary_tree_solution_10.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/connect_all_siblings_of_binary_tree_solution_11.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/connect_all_siblings_of_binary_tree_solution_12.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/connect_all_siblings_of_binary_tree_solution_13.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/connect_all_siblings_of_binary_tree_solution_14.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/connect_all_siblings_of_binary_tree_solution_15.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/connect_all_siblings_of_binary_tree_solution_16.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/connect_all_siblings_of_binary_tree_solution_17.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/connect_all_siblings_of_binary_tree_solution_2.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/connect_all_siblings_of_binary_tree_solution_3.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/connect_all_siblings_of_binary_tree_solution_4.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/connect_all_siblings_of_binary_tree_solution_5.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/connect_all_siblings_of_binary_tree_solution_6.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/connect_all_siblings_of_binary_tree_solution_7.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/connect_all_siblings_of_binary_tree_solution_8.pngis excluded by!**/*.pngdatastructures/trees/binary/images/solutions/connect_all_siblings_of_binary_tree_solution_9.pngis excluded by!**/*.png
📒 Files selected for processing (6)
algorithms/graphs/alien_dictionary/test_alien_dictionary.pydatastructures/trees/binary/README.mddatastructures/trees/binary/node.pydatastructures/trees/binary/test_utils.pydatastructures/trees/binary/tree/binary_tree.pydatastructures/trees/binary/utils.py
🧰 Additional context used
🧬 Code graph analysis (1)
datastructures/trees/binary/test_utils.py (2)
datastructures/trees/binary/utils.py (2)
connect_all_siblings(74-116)connect_all_siblings_ptr(119-164)datastructures/trees/binary/node.py (1)
BinaryTreeNode(6-216)
🪛 LanguageTool
datastructures/trees/binary/README.md
[style] ~78-~78: This phrase is redundant. Consider writing “filled”.
Context: ...binary tree in which all the levels are completely filled with nodes, and all leaf nodes (nodes w...
(COMPLETELY_ANNIHILATE)
[style] ~118-~118: Using many exclamation marks might seem excessive (in this case: 23 exclamation marks for a text that’s 7222 characters long)
Context: ...siblings_of_binary_tree_solution_1.png) 
🔇 Additional comments (5)
algorithms/graphs/alien_dictionary/test_alien_dictionary.py (1)
76-78: LGTM!Standard test runner boilerplate addition enabling direct script execution.
datastructures/trees/binary/README.md (1)
68-144: Documentation looks comprehensive.The new section clearly explains the problem, constraints, algorithm steps, and complexity analysis. The step-by-step walkthrough with solution images will be helpful for understanding the two-pointer approach.
datastructures/trees/binary/utils.py (2)
97-116: LGTM - BFS implementation is correct.The level-order traversal correctly collects all nodes and connects them via
nextpointers in the expected order.
138-164: LGTM - Two-pointer implementation is correct.The algorithm cleverly relies on previously-set
nextpointers to traverse the current level whilelastbuilds the next-pointer chain. Space complexity is O(1) as claimed.datastructures/trees/binary/node.py (1)
206-213:__eq__does not comparenextpointers.The equality check compares
data,left, andrightbut ignoresnext. If tests rely onassertEqualto verify sibling connections, they won't actually validate thenextpointer chain.Consider whether this is intentional (structural equality ignoring sibling connections) or if
nextshould be included:# If next should be compared: if ( other.data == self.data and self.left == other.left and self.right == other.right and self.next == other.next # Add this ):Note: If you add
nextcomparison, ensure thenextchain is acyclic to avoid infinite recursion.
| class ConnectAllSiblingsTestCase(unittest.TestCase): | ||
| @parameterized.expand(CONNECT_ALL_SIBLINGS_TEST_CASES) | ||
| def test_connect_all_siblings(self, root: BinaryTreeNode, expected: BinaryTreeNode): | ||
| actual = connect_all_siblings(root) | ||
| self.assertEqual(expected, actual) | ||
|
|
||
| @parameterized.expand(CONNECT_ALL_SIBLINGS_TEST_CASES) | ||
| def test_connect_all_siblings_ptr( | ||
| self, root: BinaryTreeNode, expected: BinaryTreeNode | ||
| ): | ||
| actual = connect_all_siblings_ptr(root) | ||
| self.assertEqual(expected, actual) |
There was a problem hiding this comment.
Test isolation issue: input tree is mutated between tests.
Both connect_all_siblings and connect_all_siblings_ptr modify the tree in-place by setting next pointers. When parameterized.expand provides the same root object to both test methods, the first test mutates the tree, and the second test receives the already-modified tree.
Suggested fix: Create fresh tree instances for each test
+def create_test_tree():
+ return BinaryTreeNode(
+ data=100,
+ left=BinaryTreeNode(
+ data=50, left=BinaryTreeNode(data=25), right=BinaryTreeNode(data=75)
+ ),
+ right=BinaryTreeNode(
+ data=200, left=BinaryTreeNode(data=300), right=BinaryTreeNode(data=10)
+ ),
+ )
+
class ConnectAllSiblingsTestCase(unittest.TestCase):
- @parameterized.expand(CONNECT_ALL_SIBLINGS_TEST_CASES)
- def test_connect_all_siblings(self, root: BinaryTreeNode, expected: BinaryTreeNode):
- actual = connect_all_siblings(root)
- self.assertEqual(expected, actual)
+ def test_connect_all_siblings(self):
+ root = create_test_tree()
+ result = connect_all_siblings(root)
+ # verify next chain...
- @parameterized.expand(CONNECT_ALL_SIBLINGS_TEST_CASES)
- def test_connect_all_siblings_ptr(
- self, root: BinaryTreeNode, expected: BinaryTreeNode
- ):
- actual = connect_all_siblings_ptr(root)
- self.assertEqual(expected, actual)
+ def test_connect_all_siblings_ptr(self):
+ root = create_test_tree()
+ result = connect_all_siblings_ptr(root)
+ # verify next chain...🤖 Prompt for AI Agents
In @datastructures/trees/binary/test_utils.py around lines 229 - 240, The tests
in ConnectAllSiblingsTestCase mutate the input trees (next pointers), causing
cross-test pollution when parameterized.expand reuses the same root objects; fix
by cloning or reconstructing a fresh tree for each test invocation (e.g., deep
copy the provided root or rebuild from a serialized representation) inside both
test_connect_all_siblings and test_connect_all_siblings_ptr before calling
connect_all_siblings or connect_all_siblings_ptr so each test operates on an
unmodified tree.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
github-actions
bot
force-pushed
the
feat/datastructures-binary-tree-siblings
branch
from
8aa6bd5 to
14fde9b
Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
datastructures/trees/binary/utils.py (2)
74-116: Algorithm logic is correct.The BFS-based approach correctly collects nodes level-by-level and links them sequentially. Minor typo on line 90: "travers" should be "traverse".
📝 Typo fix
- Time complexity is O(n) as we travers all the nodes in the tree. + Time complexity is O(n) as we traverse all the nodes in the tree.
119-164: Two-pointer algorithm is correct for perfect binary trees.The logic correctly builds the
nextchain level-by-level by processing nodes in the order they're connected. Edge cases (None root, single-node tree) are handled properly.Minor docstring formatting issue on line 130 with extra leading whitespace.
📝 Docstring formatting fix
- This performs a level order traversal using two pointers to traverse the tree utilizing constant space in the process - making it efficient in terms of space. However, this incurs a time complexity of O(n) as all the nodes in the tree - have to be traversed from the root to make the connections + This performs a level order traversal using two pointers to traverse the tree utilizing constant space in the process + making it efficient in terms of space. However, this incurs a time complexity of O(n) as all the nodes in the tree + have to be traversed from the root to make the connections
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
datastructures/trees/binary/README.mddatastructures/trees/binary/utils.py
🧰 Additional context used
🧬 Code graph analysis (1)
datastructures/trees/binary/utils.py (1)
datastructures/trees/binary/node.py (1)
BinaryTreeNode(6-216)
🪛 LanguageTool
datastructures/trees/binary/README.md
[style] ~78-~78: This phrase is redundant. Consider writing “filled”.
Context: ...binary tree in which all the levels are completely filled with nodes, and all leaf nodes (nodes w...
(COMPLETELY_ANNIHILATE)
[style] ~118-~118: Using many exclamation marks might seem excessive (in this case: 23 exclamation marks for a text that’s 7223 characters long)
Context: ...siblings_of_binary_tree_solution_1.png) 
🔇 Additional comments (2)
datastructures/trees/binary/utils.py (1)
1-2: LGTM!The imports are appropriate for the new BFS-based (
Deque,List,deque) and pointer-based utility functions.datastructures/trees/binary/README.md (1)
68-144: Documentation is well-structured and accurate.The algorithm description correctly matches the
connect_all_siblings_ptrimplementation, and the complexity analysis is accurate. The step-by-step breakdown clearly explains the two-pointer approach.Note: The static analysis hint about "excessive exclamation marks" is a false positive—those are Markdown image syntax characters (
), not actual exclamation points.
…rison of entire tree
Describe your change:
Connect all siblings of a perfect binary tree
Checklist:
Fixes: #{$ISSUE_NO}.Summary by CodeRabbit
New Features
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.